Skip to content

Java: automodel application mode: use endpoint class like in framework mode#13886

Merged
kaeluka merged 9 commits into
mainfrom
kaeluka/java-automodel-variadic-args
Aug 16, 2023
Merged

Java: automodel application mode: use endpoint class like in framework mode#13886
kaeluka merged 9 commits into
mainfrom
kaeluka/java-automodel-variadic-args

Conversation

@kaeluka

@kaeluka kaeluka commented Aug 4, 2023

Copy link
Copy Markdown

There now is a new endpoint class in application mode that mirrors the one we recently added to framework mode. The new class additionally has proper support for vararg arrays.

You'll find some documentation of how the class behaves in the qll file.
The test case in Test.java shows how varargs are treated.

Questions for Reviewers

  • For varargs arguments, we only use the first argument passed as endpoint (example). This is easy and avoids redundancy. We're not forced to do it that way, but it makes sense to me. Otherwise, we'd push complexity to the the classification script (eg., it might decide to use competing classifications for a given MaD model) and to the triage tooling. Do you agree?

fyi, @jhelie

@github-actions github-actions Bot added the Java label Aug 4, 2023
@kaeluka kaeluka added the no-change-note-required This PR does not need a change note label Aug 4, 2023
@kaeluka kaeluka force-pushed the kaeluka/java-automodel-variadic-args branch from 57cd4c1 to d31e48d Compare August 7, 2023 07:53
@kaeluka kaeluka force-pushed the kaeluka/java-automodel-variadic-args branch from 6f746c7 to e1a5eba Compare August 7, 2023 10:19
@kaeluka kaeluka force-pushed the kaeluka/java-automodel-variadic-args branch from 4906015 to 3433437 Compare August 7, 2023 12:15
@kaeluka kaeluka marked this pull request as ready for review August 7, 2023 14:26
@kaeluka kaeluka requested a review from a team as a code owner August 7, 2023 14:26
@kaeluka

kaeluka commented Aug 8, 2023

Copy link
Copy Markdown
Author

@jcogs33, are you able to take a look? CI Checks are ✅

@jcogs33

jcogs33 commented Aug 8, 2023

Copy link
Copy Markdown
Contributor

@jcogs33, are you able to take a look? CI Checks are ✅

Sure, I'll try to take a look sometime tomorrow (2023/08/09).

@jcogs33 jcogs33 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a couple minor questions, otherwise looks reasonable to me. You might want a second pair of 👀 since I'm not sure if I'm familiar enough with the automodel code (as my questions might show 🙂).

For varargs arguments, we only use the first argument passed as endpoint (example). This is easy and avoids redundancy. We're not forced to do it that way, but it makes sense to me. Otherwise, we'd push complexity to the the classification script (eg., it might decide to use competing classifications for a given MaD model) and to the triage tooling. Do you agree?

I agree that just using the the first varargs argument as endpoint sounds like the best solution.

Comment thread java/ql/src/Telemetry/AutomodelApplicationModeExtractCandidates.ql Outdated
Comment thread java/ql/src/Telemetry/AutomodelApplicationModeExtractNegativeExamples.ql Outdated
Comment thread java/ql/src/Telemetry/AutomodelApplicationModeExtractPositiveExamples.ql Outdated
Comment thread java/ql/src/Telemetry/AutomodelApplicationModeCharacteristics.qll
@kaeluka

kaeluka commented Aug 14, 2023

Copy link
Copy Markdown
Author

Thanks for the thorough review 🥳 I think I've addressed your comments :)

@jcogs33 jcogs33 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me :)

@kaeluka kaeluka merged commit 20254c3 into main Aug 16, 2023
@kaeluka kaeluka deleted the kaeluka/java-automodel-variadic-args branch August 16, 2023 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Java no-change-note-required This PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants